Repository navigation
refactor(api)!: migrate API errors to RFC 7807 ProblemDetail - #149
hamdirahalpartner-del wants to merge 1 commit into
Conversation
|
|
Code coverage overviewLanguages: Java Java / code-coverage/jacoco
View coverage of the 5 most impacted files.
ℹ️ Calculations represent line coverage. Results may change as calculations are updated. Updated |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved moderate findings affect error contracts, status codes, and instance URIs, with related specification and documentation updates pending.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (4)
What changed in this PR
This PR migrates REST API errors from ErrorResponse to Spring ProblemDetail/RFC 7807 while retaining legacy fields and updating contracts, tests, and documentation.
Changes:
- Refactors centralized exception handling with timestamps and legacy fields.
- Updates controller OpenAPI schemas and error-response tests.
- Documents the new error contract and migration considerations.
| File | Reviewed changes and final review notes |
|---|---|
src/test/java/com/decathlon/idp_core/infrastructure/adapters/api/handler/ApiExceptionHandlerTest.java |
Updates handler tests. nit (2 votes): pass the request through a framework override and assert ProblemDetail.instance. |
src/test/java/com/decathlon/idp_core/infrastructure/adapters/api/controller/EntityTemplateControllerTest.java |
Updates error assertions; no final review comments. |
src/test/java/com/decathlon/idp_core/infrastructure/adapters/api/controller/EntityDynamicMappingControllerTest.java |
Updates problem media-type assertions; no final review comments. |
src/test/java/com/decathlon/idp_core/infrastructure/adapters/api/controller/EntityControllerTest.java |
Updates error assertions; no final review comments. |
src/main/java/com/decathlon/idp_core/infrastructure/adapters/api/handler/ApiExceptionHandler.java |
Implements ProblemDetail mappings. moderate (3 votes): inherited framework mappings can omit legacy fields (also at lines 107 and 543). moderate (1 vote): preserve Spring’s supplied status code for HandlerMethodValidationException. moderate (1 vote): strip only a leading uri= prefix when constructing the instance URI. |
src/main/java/com/decathlon/idp_core/infrastructure/adapters/api/controller/InboundWebhookConfigurationController.java |
Updates ProblemDetail OpenAPI schemas; no final review comments. |
src/main/java/com/decathlon/idp_core/infrastructure/adapters/api/controller/EntityTemplateController.java |
Updates response annotations. nit (3 votes): regenerate docs/src/static/swagger.yaml so the published specification matches the generated contract. |
src/main/java/com/decathlon/idp_core/infrastructure/adapters/api/controller/EntityGraphController.java |
Updates ProblemDetail OpenAPI schemas; no final review comments. |
src/main/java/com/decathlon/idp_core/infrastructure/adapters/api/controller/EntityDynamicMappingController.java |
Updates ProblemDetail OpenAPI schemas; no final review comments. |
src/main/java/com/decathlon/idp_core/infrastructure/adapters/api/controller/EntityController.java |
Updates ProblemDetail OpenAPI schemas; no final review comments. |
src/main/java/com/decathlon/idp_core/infrastructure/adapters/api/controller/AuditController.java |
Updates ProblemDetail OpenAPI schemas; no final review comments. |
docs/src/contributing/code/exception-handling.md |
Documents the new error contract. nit (1 vote): update the architecture diagram’s stale ErrorResponse reference. nit (2 votes): address the breaking media-type change in the PR title or justify its classification. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
e012ca6 to
3896ffd
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Critical and moderate issues remain in exception handling and the published OpenAPI contract.
9 open findings
Return ProblemDetail for generic API failures · New Advertise application/problem+json in response annotations · New Document ProblemDetail extensions at the schema root · New Remove CI-only localhost token URL from Swagger · New Delegate unreadable-body handling to native ProblemDetail support · New Include instance in domain error responses · New Fix punctuation and spacing in error-format documentation · New Regenerate OpenAPI spec for ProblemDetail responses Mark breaking error media type change in the PR title
2 resolved since last review
🧠 Review effort: Lite
6d3c0a7 to
ff5e520
Compare
7711211 to
2820676
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Resolve the documented OpenAPI inconsistencies and migrate the remaining legacy security error responses.
6 open issues
Apply OpenAPI customization to grouped documents · New Specify problem media type for the 400 response · New Migrate principal endpoint security errors to ProblemDetail · New Document ProblemDetail extensions at the schema root Update ProblemDetail schema with root-level error fields · New Mark breaking error media type change in the PR title
7 resolved since last review
Return ProblemDetail for generic API failures Include instance in domain error responses Delegate unreadable-body handling to native ProblemDetail support Remove CI-only localhost token URL from Swagger Advertise application/problem+json in response annotations Fix punctuation and spacing in error-format documentation Regenerate OpenAPI spec for ProblemDetail responses
Review effort: Lite
foukou19
left a comment
There was a problem hiding this comment.
It looks like the legacy ErrorResponse type hasn't been completely removed yet.
Could you please check these remaining files?
src/main/java/com/decathlon/idp_core/infrastructure/adapters/common/model/ErrorResponse.java
src/main/java/com/decathlon/idp_core/infrastructure/adapters/ingestion/exception_handler/WebhookExceptionHandlerHelper.java
Thanks!
beb8184 to
cf2bc52
Compare
There was a problem hiding this comment.
🟡 Changes recommended
ProblemDetail documentation and OpenAPI coverage are incomplete, and the webhook route still uses the legacy error contract.
7 open issues
Scope migration claim to MVC or migrate webhook errors · New Add ProblemDetail content to inbound webhook errors · New Migrate Camel webhook errors or narrow migration scope · New Migrate principal endpoint security errors to ProblemDetail Update contributor docs from ErrorResponse to ProblemDetail · New Remove obsolete ErrorResponse schema from API contract · New Mark breaking error media type change in the PR title
4 resolved since last review
Review effort: Lite
cf2bc52 to
00949e3
Compare
|
00949e3 to
fc410c3
Compare
Signed-off-by: hamdirahalpartner-del <hamdi.rahal.partner@decathlon.com>
aca1807 to
0bdbf59
Compare






PR Description
What this PR Provides
Fixes
Review
The reviewer must double-check these points:
!after the type/scope to identify the breakingchange in the release note and ensure we will release a major version.
How to test
Initial state: application running normally, no specific data setup required.
What and how to test:
GET /api/v1/entity_templates/{identifier}with a non-existent identifier (404), orPOST /api/v1/entity_templateswith an invalid payload (400)../gradlew test --tests "*ApiExceptionHandlerTest*"along with the impacted controller tests (EntityControllerTest,EntityTemplateControllerTest,EntityDynamicMappingControllerTest).docs/src/static/swagger.yaml/ Swagger UI to confirm error responses now reference theProblemDetailschema.Expected results:
ProblemDetailformat:{"type": ..., "title": ..., "status": ..., "detail": ..., "instance": ...}(Content-Typeapplication/problem+json), replacing the previous{"error": ..., "error_description": ...}format.Breaking changes (if any)
Context of the Breaking Change
The API error response format has been migrated from the internal
ErrorResponseclass ({"error": "...", "error_description": "..."}) to Spring's RFC 7807ProblemDetailstandard (application/problem+json), for all errors (400, 404, 409, 500, etc.) across all endpoints.Result of the Breaking Change
Clients consuming the IDP-Core API that parse error bodies on the
error/error_descriptionfields must be updated to read the newProblemDetailformat:type,title,status,detail,instance. TheContent-Typeof error responses changes toapplication/problem+json.